-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more tests for single distributions #621
Conversation
xukai92
commented
Dec 10, 2018
•
edited
Loading
edited
- Uni
- Multi
- Matrix
I appreciate that this is a WIP, but I've added a few comments anyway. Is there an obvious way to construct some |
By |
Fair point. If we're only considering simple distributions in this PR then we are essentially just trying to ensure that Turing hasn't screwed anything up and, therefore, can just compare against the |
Yes. Actually I'd prefer to add them into this |
@mohamed82008 I kept seeing numerical errors from Bijectors.jl (https://travis-ci.org/TuringLang/Turing.jl/jobs/474636293#L873) when sampling for single Dirichlet distribution with NUTS. I didn't manage to figure out what happens here - any idea? |
Sorry this is irrelevant. |
I found a possible cause and created an issue in Bijectors.jl (see TuringLang/Bijectors.jl#12). |
@yebai @willtebbutt As the numerical issue on Dirichlet is a separate issue, can we merge this PR if there is no objection? |
@mohamed82008 Could you take a look at the numerical issues identified by this PR? Ideally, we should fix them before merging this PR. |
Sure thing. I was planning to take a look a while back but forgot! I will see if I can reproduce the error first. |
All tests pass on my machine. |
I've seen |
I guess it's because of the end-of-window update that tunes the step size to a very high value (https://travis-ci.org/TuringLang/Turing.jl/jobs/475012349#L780). Let me cap that as well. |
@xukai92 all the tests are now passing. Do you want to resolve Mohamed's comment before this PR gets merged? |
@yebai Thanks for the reminding - resolved. |
@xukai92 I don't want to too much additional work as this PR looks great, but there do appear to be a few distributions missing from this list e.g. |
@willtebbutt I added more dists now! |
Samples from |
8cb6823
to
42d4d4a
Compare
Rebased on master. |
Let's merge this one if not objections |
@yebai It would be nice to merge this before the other small PRs which are also ready to merge. |
@mohamed82008 I merged it :) |